Skip to content

feat(cosim): add the full-capture observable golden, and close the rung-0 gate - #436

Merged
doublegate merged 1 commit into
mainfrom
feat/v2.4.2-observable-golden
Aug 21, 2026
Merged

feat(cosim): add the full-capture observable golden, and close the rung-0 gate#436
doublegate merged 1 commit into
mainfrom
feat/v2.4.2-observable-golden

Conversation

@doublegate

Copy link
Copy Markdown
Owner

The rung-0 gate is "feed RustyNES's golden back in as if it were the DUT and get 0 divergences from every diff CLI." For the checkpoint format it was not implementable, and that was found by trying to build it rather than by reading the spec.

The CSV cannot re-derive the checkpoints

irq.csv carries 23 columns, and neither pc nor put_cycle_post is among them — two of the nine fields Observable folds into the hash are simply absent. Verified by parsing the header against CycleRecord rather than assumed: six of the struct's 29 fields never reach the CSV, and two of those six are observable.

So an external testbench reading the CSV cannot reproduce the numbers in the .ckpt.bin beside it. The self-diff had nothing to run on.

<stem>.obs.bin closes that — repeated 16-byte records in the same wire encoding the hash folds, headerless for the same reason the checkpoint stream is. It is also the input a re-run of a located window consumes, so it would have been needed regardless: checkpoints say which 4096-cycle window differs, and the re-run needs the records in it.

Additive on purpose. The CSV is untouched, because scripts/irq_trace_cross_diff.py and the committed golden/irq_trace/*.csv both depend on its shape, and a format bump there to serve a new consumer would be paid for by every existing one.

Decode refuses what it does not understand

A non-zero reserved pad byte, an undefined flag bit, an unknown bus-access code, a short record, a stream length that is not a multiple of 16. Not pedantry: reading a record from a newer producer as though nothing had changed is how a format divergence gets reported as a DUT divergence — the single most expensive way to be wrong at this rung, because the DUT is the thing under suspicion and the testbench is not.

The stream is emitted even when the checkpoints are refused for overflow. A hash over a truncated trace claims a coverage it does not have; the records themselves are just records.

Measured across the repository boundary

$ tb/selfdiff_check.sh          # in the RustyNES_MiSTer checkout
1/3 exporting goldens from the oracle
2/3 re-deriving checkpoints on this side
re-derived 22 checkpoints from 89335 records
3/3 comparing
checkpoints match: 22 compared, 0 divergences

negative control: one flipped bit must be located
DIVERGED at checkpoint 10
  window to re-run with full capture: cycles (40967, 45063]  (4096 cycles)

rung 0 self-diff: agreement recognised, and disagreement located

89,335 records of AccuracyCoin, re-derived in C++ from .obs.bin alone, hashing to byte-identical checkpoints. The negative control runs in the same invocation, because a positive control alone is satisfiable by a comparison that always agrees — a harness that only ever says "match" passes everything.

Also verified: a malformed record and a truncated stream are both refused with exit 3 rather than hashed.

nes_golden_export::main crossed clippy's too_many_lines while this landed. Extracted write_irq_artifacts rather than allowing the lint — main was doing argument parsing, running, and five writes, and the extracted function is where the one-take-three-artifacts constraint is now documented.

Gates

Core untouched — no file under crates/rustynes-{cpu,ppu,apu,mappers,core} changes — so AccuracyCoin 141/141 and nestest 0-diff hold by construction.

fmt (workspace and the excluded crate) · clippy on both · rustdoc -D warnings on both · markdownlint.

126 workspace suites / 2223 passed / 0 failed, plus 4 excluded-crate suites / 42 passed (was 39).

The matching C++ half is in the sibling repository (tb/selfdiff.cpp, tb/selfdiff_check.sh), pushed alongside.

…ng-0 gate

The rung-0 gate is "feed RustyNES's golden back in as if it were the DUT and
get 0 divergences from every diff CLI". It was not implementable for the
checkpoint format, and that was found by trying to build it rather than by
reading the spec.

THE CSV CANNOT RE-DERIVE THE CHECKPOINTS

irq.csv carries 23 columns. Neither pc nor put_cycle_post is among them -- two
of the nine fields Observable folds into the hash are simply absent from the
file. Verified by parsing the header against CycleRecord rather than assumed:
six of the struct's 29 fields never reach the CSV, and two of those six are
observable.

So an external testbench reading the CSV cannot reproduce the numbers in the
.ckpt.bin sitting beside it. The self-diff had nothing to run on.

<stem>.obs.bin closes that: repeated 16-byte records in the SAME wire encoding
the hash folds, headerless for the same reason the checkpoint stream is -- the
manifest already records the ROM hash, the seed and the frame count, and a
second place to state them is a second place for them to disagree.

Additive on purpose. The CSV is untouched, because scripts/irq_trace_cross_diff.py
and the committed golden/irq_trace/*.csv both depend on its shape, and a format
bump there to serve a new consumer would be paid for by every existing one.

The stream is also what a re-run of a located window consumes, so it would have
been needed regardless. Checkpoints say WHICH 4096-cycle window differs; the
re-run needs the records in it.

DECODE REFUSES WHAT IT DOES NOT UNDERSTAND

Observable::decode rejects a non-zero reserved pad byte, an undefined flag bit,
an unknown bus-access code, a short record, and a stream length that is not a
multiple of 16. That is not pedantry. Reading a record from a newer producer as
though nothing had changed is how a FORMAT divergence gets reported as a DUT
divergence -- and at this rung that is the single most expensive way to be
wrong, because the DUT is the thing under suspicion and the testbench is not.

The stream is emitted even when the checkpoints are REFUSED for overflow. A
hash over a truncated trace claims a coverage it does not have; the records
themselves are just records, and are worth keeping for a re-run.

MEASURED ACROSS THE REPOSITORY BOUNDARY, NOT ONLY IN UNIT TESTS

  $ tb/selfdiff_check.sh          (in the RustyNES_MiSTer checkout)
  1/3 exporting goldens from the oracle
  2/3 re-deriving checkpoints on this side
  re-derived 22 checkpoints from 89335 records
  3/3 comparing
  checkpoints match: 22 compared, 0 divergences

  negative control: one flipped bit must be located
  DIVERGED at checkpoint 10
    window to re-run with full capture: cycles (40967, 45063]  (4096 cycles)

  rung 0 self-diff: agreement recognised, and disagreement located

89,335 records of AccuracyCoin, re-derived in C++ from .obs.bin alone, hashing
to byte-identical checkpoints. The negative control runs in the SAME
invocation, because a positive control alone is satisfiable by a comparison
that always agrees -- and a harness that only ever says "match" passes
everything.

Also verified: a malformed record and a truncated stream are both refused with
exit 3 rather than hashed.

nes_golden_export's main() crossed clippy's too_many_lines while this landed.
Extracted write_irq_artifacts rather than allowing the lint -- main was doing
argument parsing, running, and five writes, and the extracted function is where
the one-take-three-artifacts constraint is now documented.

Gates. The emulation core is untouched; no file under
crates/rustynes-{cpu,ppu,apu,mappers,core} changes, so AccuracyCoin 141/141 and
nestest 0-diff hold by construction.

  fmt (workspace and the excluded crate), clippy on both, rustdoc -D warnings
  on both, markdownlint on both changed documents.

  126 workspace suites / 2223 passed / 0 failed
    4 excluded-crate suites /   42 passed / 0 failed  (was 39)
Copilot AI lite review requested due to automatic review settings August 21, 2026 02:00
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 723eb959-2868-4745-ab22-3885e9e00073


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@github-actions

Copy link
Copy Markdown

Antigravity review (Gemini via Ultra)

Adds a full-capture observable stream (.obs.bin) and its decoder to enable an external testbench to re-derive checkpoint hashes without relying on the incomplete CSV output.

Blocking issues

  • Data loss in FFI API (crates/rustynes-cosim/src/lib.rs): rn_write_observables calls oracle.take_irq_artifacts(), which permanently consumes the trace and drops the CSV and checkpoint data. Any subsequent FFI calls to retrieve those other artifacts will silently fail with -4 (unarmed). If the C API is meant to write multiple artifacts, it needs a unified rn_write_irq_artifacts function (similar to the fix applied in nes_golden_export.rs) to avoid discarding state.

Suggestions

  • crates/rustynes-cosim/src/bin/nes_golden_export.rs: Emitting .obs.bin and then immediately panicking on a checkpoints error leaves the directory in an inconsistent state (an orphaned .obs.bin with no manifest.txt). Consider handling the overflow error gracefully to write the manifest, or explicitly note that partial artifacts are expected upon panic.
  • crates/rustynes-cosim/src/checkpoint.rs: bytes.len().is_multiple_of(ENCODED_LEN) requires Rust 1.73+. If the project's MSRV is lower, use bytes.len() % ENCODED_LEN != 0 instead.

Nitpicks

  • crates/rustynes-cosim/src/checkpoint.rs: b[0..8].try_into().expect("8 bytes") is safe but verbose; a simple .unwrap() is idiomatic for slices statically known to be the correct size.

Automated first-pass review by agy on a self-hosted runner -- not a human review.

@doublegate
doublegate merged commit a057504 into main Aug 21, 2026
27 of 28 checks passed
@doublegate
doublegate deleted the feat/v2.4.2-observable-golden branch August 21, 2026 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants